✨ Import the reference client stack (client core, hub-shim, UI, harnesses, demo)#35
Open
ibolton336 wants to merge 4 commits into
Open
✨ Import the reference client stack (client core, hub-shim, UI, harnesses, demo)#35ibolton336 wants to merge 4 commits into
ibolton336 wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe PR imports a reference client stack with shared AgentRun and ACP clients, a hub-shim service, mock and Goose harnesses, a PatternFly UI, Kubernetes deployment resources, demo workflows, and ADR documentation. ChangesReference client contracts and transports
Hub-shim and harness runtime
Browser UI and cluster deployment
Demo workflows and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 20, 2026 18:34
9b4a171 to
310f00c
Compare
Member
Author
|
Rebased onto main (post #33/#34/#37) — no conflicts — plus a follow-up commit aligning the stack with what merged while this sat:
All packages typecheck/build green (tsc + vite). @djzager ready for review when you get a chance. 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
clients/hack/demo-up.sh (1)
83-84: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse a project-local directory for state files instead of
/tmp. Hardcoding predictable paths in the world-writable/tmpdirectory exposes the scripts to local symlink attacks (CWE-377) and pollutes global state. Consider creating a project-local directory likeSTATE_DIR="$ROOT/.demo"and writing PID and log files there.
clients/hack/demo-up.sh#L83-L84: redirect the port-forward log and pid to$STATE_DIR/demo-hub-pf....clients/hack/demo-up.sh#L110-L111: redirect the hub-shim log and pid to$STATE_DIR/demo-hub-shim....clients/hack/demo-up.sh#L128-L129: redirect the ui log and pid to$STATE_DIR/demo-ui....clients/hack/demo-down.sh#L8-L8: read the pidfiles from$ROOT/.demo/demo-$name.pidinstead of/tmp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/hack/demo-up.sh` around lines 83 - 84, Replace world-writable /tmp state paths with a project-local STATE_DIR under $ROOT/.demo, creating it before use. In clients/hack/demo-up.sh lines 83-84, 110-111, and 128-129, write each demo log and PID file beneath STATE_DIR; in clients/hack/demo-down.sh line 8, read the demo-$name.pid files from $ROOT/.demo instead of /tmp.Source: Linters/SAST tools
clients/deploy/ui/nginx.conf (1)
12-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueStartup-time DNS resolution of the gateway upstream can crashloop the UI pod.
proxy_passto the bareagentic-gatewayhostname is resolved once at nginx startup. If theagentic-gatewayService object isn't present when this pod starts, nginx fails with "host not found in upstream" and crashloops. Applyinggateway.yamlandui.yamltogether via the kustomization usually avoids this, but ordering isn't guaranteed. For resilience, resolve lazily via aresolver+ variable upstream (appending$request_urito preserve the path).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/deploy/ui/nginx.conf` around lines 12 - 13, Update the nginx location /api/ configuration to resolve agentic-gateway lazily using a resolver directive and a variable-based proxy_pass, appending $request_uri so the original request path and query are preserved; avoid the bare static upstream form that performs DNS resolution during nginx startup.clients/deploy/manifests/gateway.yaml (1)
59-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden both Deployments with an explicit container
securityContext. Both containers run with the default security context (root-capable, privilege escalation allowed), flagged by Trivy (KSV-0014/KSV-0118) and Checkov (CKV_K8S_20/CKV_K8S_23). The gateway holdssecrets: getRBAC, so this is worth tightening even for an interim deployment. Apply the always-safe subset below;readOnlyRootFilesystem: trueadditionally needs writableemptyDirmounts (e.g./tmp,/var/cache/nginxfor the UI) so add it separately once volumes are wired.
clients/deploy/manifests/gateway.yaml#L59-L84: add asecurityContextto thegatewaycontainer.clients/deploy/manifests/ui.yaml#L22-L40: add the samesecurityContextto theuicontainer (thenginx-unprivilegedbase already runs as uid 101, sorunAsNonRootis consistent).securityContext: runAsNonRoot: true allowPrivilegeEscalation: false capabilities: drop: ["ALL"] seccompProfile: type: RuntimeDefault🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/deploy/manifests/gateway.yaml` around lines 59 - 84, The gateway container in clients/deploy/manifests/gateway.yaml lines 59-84 and the UI container in clients/deploy/manifests/ui.yaml lines 22-40 both require the same explicit container securityContext: set runAsNonRoot, disable privilege escalation, drop all capabilities, and use the RuntimeDefault seccomp profile. Do not enable readOnlyRootFilesystem until the required writable emptyDir mounts are added.Source: Linters/SAST tools
clients/ui/src/app.css (1)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated
word-break: break-word.The
word-break: break-wordproperty is deprecated in modern CSS. Useoverflow-wrap: break-wordinstead to achieve the same result in a standard-compliant way.♻️ Proposed refactor
white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/ui/src/app.css` around lines 59 - 61, In the affected CSS rule in app.css, replace the deprecated word-break: break-word declaration with overflow-wrap: break-word, preserving the existing white-space behavior and other declarations.Source: Linters/SAST tools
clients/deploy/README.md (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language identifier to the fenced code block.
As suggested by static analysis, fenced code blocks should have a language specified. Since this is an ASCII architecture diagram, using
textis appropriate.♻️ Proposed refactor
-``` +```text browser ── ingress/route (TLS + SSO) ── agentic-ui (nginx, static SPA) │ /api + WS, same-origin agentic-gateway (SA + RBAC) │ CRs via k8s API · pod :4000 via service DNS agentic-controller / Agent Sandbox / sandbox pods</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@clients/deploy/README.mdaround lines 8 - 14, Update the fenced architecture
diagram code block in the README to include the text language identifier,
changing the opening fence to ```text while preserving the diagram content
unchanged.</details> <!-- cr-comment:v1:b79bc3a37843f16e04d4d1ac --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>clients/harness-mock/Dockerfile (1)</summary><blockquote> `1-7`: _🔒 Security & Privacy_ | _🔵 Trivial_ | _💤 Low value_ **Both harness images run as `root` (Trivy DS-0002).** Shared root cause: no `USER` directive. Since the goose harness clones arbitrary repos and executes agent tooling, dropping root is especially worthwhile there. - `clients/harness-mock/Dockerfile#L1-L7`: add `USER node` before `CMD` (ensure `/app` is readable by it). - `clients/harness-goose/Dockerfile#L28-L33`: create/own a non-root user with write access to `/workspace` and switch to it before `CMD`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/harness-mock/Dockerfile` around lines 1 - 7, Both harness Dockerfiles currently run as root because they lack a USER directive. In clients/harness-mock/Dockerfile, ensure /app is readable by the existing node user and add USER node before CMD. In clients/harness-goose/Dockerfile, create and own a non-root user with write access to /workspace, then switch to that user before CMD. ``` </details> <!-- cr-comment:v1:1f137d115f67f507714e07ba --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>clients/manifests/goose-bedrock.yaml (1)</summary><blockquote> `15-17`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Stale references to the removed simulator in both sample manifests.** This PR excludes the prototype simulator scaffolding, yet both files still cite it. - `clients/manifests/goose-bedrock.yaml#L15-L17`: drop/replace the "See simulate-controller.ts" comment. - `clients/manifests/samples.yaml#L66-L68`: reword "The controller simulator substitutes the mock ACP harness image" to describe the real-controller behavior. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/manifests/goose-bedrock.yaml` around lines 15 - 17, Remove or replace the stale “See simulate-controller.ts” reference in clients/manifests/goose-bedrock.yaml lines 15-17 while preserving the goose-provider contract comment. In clients/manifests/samples.yaml lines 66-68, reword the note about the controller simulator substituting the mock ACP harness image to accurately describe real-controller behavior. ``` </details> <!-- cr-comment:v1:581301e5b9d12d02b6377a9d --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@clients/hack/demo-up.sh:
- Line 104: Update both npm install commands in clients/hack/demo-up.sh at lines
104-104 and 125-125 to handle failures explicitly: retain or replace the
redirection with visible/logged output and append failure handling that calls
die with a context-specific npm install error for hub-shim and the UI. Ensure
set -e failures no longer terminate the script silently.In
@clients/harness-goose/Dockerfile:
- Around line 6-19: Update the GOOSE_ARCH default used by the Dockerfile
download flow to match the target platform, using Docker’s TARGETARCH mapping
where appropriate so x86_64 builds select the goose-x86_64-unknown-linux-gnu
archive while preserving the correct ARM mapping. Keep the existing download,
extraction, and version-check steps unchanged.In
@clients/packages/agentic-client/src/transport-shim/index.ts:
- Around line 98-119: Update the transport shim’s send method to pass an
AbortSignal.timeout(30_000) signal in the fetch options, ensuring stalled
requests abort after 30 seconds while preserving the existing error handling and
response validation.In
@clients/packages/agentrun-client/src/kube.ts:
- Around line 183-197: Update the sandbox pod lookup around readNamespacedPod so
only a 404 result is treated as a missing pod and falls through to the
label-selector fallback. Preserve the fallback for not-found responses, but
rethrow non-404 errors such as RBAC, network, or server failures instead of
converting them into the generic “No sandbox pod found” error; use the existing
k8sStatusCode helper or equivalent status check.
Nitpick comments:
In@clients/deploy/manifests/gateway.yaml:
- Around line 59-84: The gateway container in
clients/deploy/manifests/gateway.yaml lines 59-84 and the UI container in
clients/deploy/manifests/ui.yaml lines 22-40 both require the same explicit
container securityContext: set runAsNonRoot, disable privilege escalation, drop
all capabilities, and use the RuntimeDefault seccomp profile. Do not enable
readOnlyRootFilesystem until the required writable emptyDir mounts are added.In
@clients/deploy/README.md:
- Around line 8-14: Update the fenced architecture diagram code block in the
README to include the text language identifier, changing the opening fence toIn `@clients/deploy/ui/nginx.conf`: - Around line 12-13: Update the nginx location /api/ configuration to resolve agentic-gateway lazily using a resolver directive and a variable-based proxy_pass, appending $request_uri so the original request path and query are preserved; avoid the bare static upstream form that performs DNS resolution during nginx startup. In `@clients/hack/demo-up.sh`: - Around line 83-84: Replace world-writable /tmp state paths with a project-local STATE_DIR under $ROOT/.demo, creating it before use. In clients/hack/demo-up.sh lines 83-84, 110-111, and 128-129, write each demo log and PID file beneath STATE_DIR; in clients/hack/demo-down.sh line 8, read the demo-$name.pid files from $ROOT/.demo instead of /tmp. In `@clients/harness-mock/Dockerfile`: - Around line 1-7: Both harness Dockerfiles currently run as root because they lack a USER directive. In clients/harness-mock/Dockerfile, ensure /app is readable by the existing node user and add USER node before CMD. In clients/harness-goose/Dockerfile, create and own a non-root user with write access to /workspace, then switch to that user before CMD. In `@clients/manifests/goose-bedrock.yaml`: - Around line 15-17: Remove or replace the stale “See simulate-controller.ts” reference in clients/manifests/goose-bedrock.yaml lines 15-17 while preserving the goose-provider contract comment. In clients/manifests/samples.yaml lines 66-68, reword the note about the controller simulator substituting the mock ACP harness image to accurately describe real-controller behavior. In `@clients/ui/src/app.css`: - Around line 59-61: In the affected CSS rule in app.css, replace the deprecated word-break: break-word declaration with overflow-wrap: break-word, preserving the existing white-space behavior and other declarations.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
bb2b57cb-0172-4b89-b1ca-b2de1851834d⛔ Files ignored due to path filters (5)
clients/harness-mock/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentic-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentrun-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/hub-shim/package-lock.jsonis excluded by!**/package-lock.jsonclients/ui/package-lock.jsonis excluded by!**/package-lock.json📒 Files selected for processing (68)
README.mdchanges/unreleased/clients-reference-stack.yamlclients/.gitignoreclients/README.mdclients/deploy/README.mdclients/deploy/gateway/Dockerfileclients/deploy/manifests/gateway.yamlclients/deploy/manifests/ingress.example.yamlclients/deploy/manifests/kustomization.yamlclients/deploy/manifests/ui.yamlclients/deploy/ui/Dockerfileclients/deploy/ui/nginx.confclients/docs/DEMO.mdclients/docs/demo/real-run.yamlclients/docs/demo/skill-probe.yamlclients/hack/demo-check.shclients/hack/demo-down.shclients/hack/demo-up.shclients/harness-goose/Dockerfileclients/harness-goose/entrypoint.shclients/harness-mock/Dockerfileclients/harness-mock/package.jsonclients/harness-mock/server.mjsclients/manifests/goose-bedrock.yamlclients/manifests/samples.yamlclients/packages/agentic-client/dev/selfcheck.tsclients/packages/agentic-client/package.jsonclients/packages/agentic-client/src/acp/index.tsclients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentic-client/tsconfig.jsonclients/packages/agentrun-client/dev/demo.tsclients/packages/agentrun-client/dev/local-smoke.tsclients/packages/agentrun-client/package.jsonclients/packages/agentrun-client/src/acp.tsclients/packages/agentrun-client/src/index.tsclients/packages/agentrun-client/src/kube.tsclients/packages/agentrun-client/src/portforward.tsclients/packages/agentrun-client/src/types.tsclients/packages/agentrun-client/tsconfig.jsonclients/packages/hub-shim/dev/browser-smoke.tsclients/packages/hub-shim/dev/dial-check.tsclients/packages/hub-shim/dev/drop-check.tsclients/packages/hub-shim/package.jsonclients/packages/hub-shim/src/acp-dial.tsclients/packages/hub-shim/src/server.tsclients/packages/hub-shim/tsconfig.jsonclients/ui/.gitignoreclients/ui/README.mdclients/ui/index.htmlclients/ui/package.jsonclients/ui/src/App.tsxclients/ui/src/app.cssclients/ui/src/components/ChatPanel.tsxclients/ui/src/components/CreateRunModal.tsxclients/ui/src/components/PhaseLabel.tsxclients/ui/src/components/RunDetailPage.tsxclients/ui/src/components/RunsPage.tsxclients/ui/src/format.tsclients/ui/src/main.tsxclients/ui/src/vite-env.d.tsclients/ui/tsconfig.app.jsonclients/ui/tsconfig.jsonclients/ui/tsconfig.node.jsonclients/ui/vite.config.tsdocs/adr/0004-client-contract-and-transports.mddocs/adr/0005-platform-resolved-params.md
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 20, 2026 19:10
310f00c to
7e54526
Compare
This was referenced Jul 20, 2026
djzager
pushed a commit
that referenced
this pull request
Jul 21, 2026
Fixes #48. The `changelog` job runs under `pull_request_target` and checked out `github.event.pull_request.head.sha`, which actions/checkout now refuses for fork PRs ("Refusing to check out fork pull request code from a 'pull_request_target' workflow"). Every fork PR of type feature/bugfix/breaking failed the check — e.g. [this run](https://github.com/konveyor/agentic-controller/actions/runs/29768454525/job/88440535802) on PR #35. Since the job only diffs file names and never executes PR code, this checks out the **base** repo and fetches the PR head as a plain ref (`git fetch origin pull/<n>/head:pr-head`), then diffs `origin/<base>...pr-head`. Fork code is never checked out into the workspace, so no `allow-unsafe-pr-checkout: true` escape hatch is needed and the workflow keeps its safety guarantees. Verification: YAML validated; the diff expression is unchanged apart from `HEAD` → `pr-head`. The fix itself can only be fully exercised by a fork PR against a repo where this workflow already runs on main (pull_request_target uses the base branch's workflow definition), so the real proof will be the next fork PR after this merges. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved pull request checks for changelog fragments. * Changelog verification now more reliably compares proposed changes with the target branch. * Updated validation behavior helps ensure checks run consistently without affecting the application experience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: ibolton336 <ibolton@redhat.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Working client-side implementation of this repo's AgentRun/ACP contract, developed and verified against the live controller on minikube (Agent Sandbox v0.5.0) with both a deterministic mock agent and a real goose+Bedrock agent: - packages/agentic-client: isomorphic client core — contract types, AcpSession over WebSocket (JSON-RPC 2.0), shim HTTP transport - packages/agentrun-client: node client — AgentRun CRUD/watch, ACP endpoint resolution, port-forward tunnel (shaped for editor-extensions) - packages/hub-shim: localhost gateway serving the SHIM HTTP API v1 proposed as the Hub passthrough proxy surface (stream 2, konveyor#21) - ui: PatternFly SPA — run list, create form with platform-resolved params, streaming chat with permission round-trips (stream 3, konveyor#22-konveyor#24) - harness-mock / harness-goose: deterministic ACP test agent and the working goose agent base (stream 4 reference, konveyor#25/konveyor#26) - deploy/: in-cluster gateway + UI manifests (interim, pre-Hub) - clients/hack + docs/DEMO.md: converge/smoke/teardown scripts and the narrated end-to-end demo runbook ADRs 0004 (verified client contract and layered transports) and 0005 (platform-resolved params) continue the repo's ADR sequence. Cleared out relative to the prototype repo: the controller simulator and vendored CRD copies (the real reconcilers live here now), the rendered controller install snapshot, upstream patch files (delivered as PRs), and simulator-era docs. All packages npm-install/typecheck/build clean in the new location; the client-core selfcheck passes 17/17. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
- goose-bedrock.yaml: keyless credentialRef — the controller now exposes the whole credential Secret via envFrom (konveyor#34); drop the run-level envFrom workaround from real-run.yaml that konveyor#34 obsoleted - ADR 0004 (still proposed): pod run labels are merged reality (konveyor#34), name-based pod resolution stays mandated; complete the injected-env list with KONVEYOR_MODEL_<ROLE>_{PROVIDER,MODEL,ENDPOINT,API_KEY} - DEMO.md / demo-up.sh: sandbox restartPolicy is OnFailure since konveyor#33; refresh stale "proposed as a follow-up PR" talking points; use the now-working konveyor.io/agentrun log selector - types.ts: credentialRef.key optional, matching the CRD post-konveyor#34 - refresh stale credential/pod-label comments in hub-shim server.ts, agentrun-client kube.ts, contract index.ts, harness-goose entrypoint - READMEs: cross-reference repo-root harness/ + images/ (konveyor#33) and add them to the project-structure block Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
- transport-shim: abort REST calls after 30s (AbortSignal.timeout) so a stalled shim can't hang every RunApi call forever - agentrun-client kube.ts: only a 404 on the by-name pod lookup falls through to the label-selector fallback; RBAC/network/5xx rethrow instead of surfacing as a generic "No sandbox pod found" - harness-goose Dockerfile: derive the goose release triple from TARGETARCH (or dpkg) instead of hardcoding aarch64 — x86_64 builds fetched a binary that couldn't execute - demo-up.sh: npm install failures now die with a pointer to their log instead of silently killing the script via set -e Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 21, 2026 01:42
b5215e3 to
130a052
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
clients/packages/agentrun-client/dev/local-smoke.ts (1)
11-15: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider using
||instead of??for environment variable fallbacks.If
process.env.PORTorprocess.env.GOOSE_SERVER__SECRET_KEYare set to empty strings, the nullish coalescing operator (??) will not trigger the fallback. ForPORT,Number("")evaluates to0, which is likely unintended. Using||ensures a proper fallback for empty strings.💡 Proposed fix
const target = { host: "127.0.0.1", - port: Number(process.env.PORT ?? 4100), - secretKey: process.env.GOOSE_SERVER__SECRET_KEY ?? "localtest", + port: Number(process.env.PORT || 4100), + secretKey: process.env.GOOSE_SERVER__SECRET_KEY || "localtest", };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/packages/agentrun-client/dev/local-smoke.ts` around lines 11 - 15, Update the target configuration’s environment fallbacks for PORT and GOOSE_SERVER__SECRET_KEY to use || instead of ??, so empty-string values select the existing defaults before port conversion or secret assignment.clients/deploy/manifests/ui.yaml (1)
21-40: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit securityContext hardening to the UI container.
Lower risk than the gateway (no RBAC/secrets access), but still missing
securityContextat pod/container level. Note: if you setreadOnlyRootFilesystem: true, nginx will need writableemptyDirmounts for its cache/pid dirs (e.g./var/cache/nginx,/tmp,/var/run).🔒 Proposed fix
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: ui image: agentic-ui:dev imagePullPolicy: IfNotPresent + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] ports: - containerPort: 8080 name: httpAs per static analysis hints (Trivy KSV-0014/KSV-0118, Checkov CKV_K8S_20/CKV_K8S_23).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/deploy/manifests/ui.yaml` around lines 21 - 40, Add explicit securityContext hardening for the UI container identified by the `name: ui` entry: run it as non-root, disallow privilege escalation, drop unnecessary Linux capabilities, and set the root filesystem read-only. Add writable `emptyDir` mounts for nginx runtime/cache paths such as `/var/cache/nginx`, `/tmp`, and `/var/run` so the existing nginx process continues to function.Source: Linters/SAST tools
clients/packages/hub-shim/src/server.ts (1)
374-387: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winApplication-identity
envFromisn't markedoptional, unlike the model-provider path.
resolveModels()(Line 446) deliberately setssecretRef.optional: trueso a missing provider Secret doesn't wedge pod creation. The identity-credential injection here (Line 381) uses a baresecretRef: { name: app.identitySecret }. If anIDENTITY_SECRET_BRIDGEentry ever points at a Secret that hasn't been created yet, the run's pod will fail withCreateContainerConfigErrorinstead of degrading gracefully the way the model path does.🔧 Proposed fix
if (app.identitySecret) { - resolved.envFrom.push({ secretRef: { name: app.identitySecret } }); + resolved.envFrom.push({ secretRef: { name: app.identitySecret, optional: true } }); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/packages/hub-shim/src/server.ts` around lines 374 - 387, Update the application-identity credential injection in the credentialSources loop to mark the generated secretRef as optional, matching the optional secret reference behavior in resolveModels(). Preserve the existing identitySecret check and skip/log behavior.clients/harness-mock/Dockerfile (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning installs with a committed lockfile +
npm ci.Only
package.jsonis copied andnpm installis used, so the resolved dependency tree isn't reproducible across builds/environments. Ifclients/harness-mockhas (or gets) apackage-lock.json, copy it too and switch tonpm ci --omit=devfor deterministic, cache-friendly builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/harness-mock/Dockerfile` around lines 3 - 4, Update the clients/harness-mock Dockerfile dependency installation to copy the committed package-lock.json alongside package.json and replace npm install with npm ci --omit=dev, preserving the existing production-only installation behavior.clients/harness-goose/Dockerfile (1)
1-45: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueBoth harness images run as root. Neither Dockerfile drops privileges before
CMD; the shared fix is adding a non-rootUSERto each.
clients/harness-goose/Dockerfile#L1-L45: add a dedicated user (e.g.useradd -m agent),chown/workspaceand/usr/local/bin/agent-entrypoint.sh, thenUSER agentbeforeCMD.clients/harness-mock/Dockerfile#L1-L8: switch to the built-innodeuser (node:24-alpineships one) viaUSER nodebeforeCMD, after ensuring/appis writable by it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/harness-goose/Dockerfile` around lines 1 - 45, Both harness images currently run as root; update clients/harness-goose/Dockerfile lines 1-45 to create a dedicated agent user, grant it ownership of /workspace and /usr/local/bin/agent-entrypoint.sh, and set USER agent before CMD. Update clients/harness-mock/Dockerfile lines 1-8 to ensure /app is writable by the built-in node user and set USER node before CMD.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/deploy/manifests/gateway.yaml`:
- Around line 59-84: Harden both Deployments: in
clients/deploy/manifests/gateway.yaml lines 59-84, add pod-level runAsNonRoot
and RuntimeDefault seccompProfile, plus container-level allowPrivilegeEscalation
false, readOnlyRootFilesystem true, and drop ALL capabilities for gateway; in
clients/deploy/manifests/ui.yaml lines 21-40, add the same pod-level settings
and container-level privilege-escalation and capability restrictions for ui,
adding emptyDir mounts for nginx writable directories if enabling
readOnlyRootFilesystem.
In `@clients/hack/demo-up.sh`:
- Around line 45-54: Update the minikube image build commands in the
acp-mock-harness and goose-harness blocks to use the same explicit failure
handling as the existing npm install commands, invoking die with a clear,
image-specific message when either build fails. Preserve the current directory
changes, build arguments, and success flow.
---
Nitpick comments:
In `@clients/deploy/manifests/ui.yaml`:
- Around line 21-40: Add explicit securityContext hardening for the UI container
identified by the `name: ui` entry: run it as non-root, disallow privilege
escalation, drop unnecessary Linux capabilities, and set the root filesystem
read-only. Add writable `emptyDir` mounts for nginx runtime/cache paths such as
`/var/cache/nginx`, `/tmp`, and `/var/run` so the existing nginx process
continues to function.
In `@clients/harness-goose/Dockerfile`:
- Around line 1-45: Both harness images currently run as root; update
clients/harness-goose/Dockerfile lines 1-45 to create a dedicated agent user,
grant it ownership of /workspace and /usr/local/bin/agent-entrypoint.sh, and set
USER agent before CMD. Update clients/harness-mock/Dockerfile lines 1-8 to
ensure /app is writable by the built-in node user and set USER node before CMD.
In `@clients/harness-mock/Dockerfile`:
- Around line 3-4: Update the clients/harness-mock Dockerfile dependency
installation to copy the committed package-lock.json alongside package.json and
replace npm install with npm ci --omit=dev, preserving the existing
production-only installation behavior.
In `@clients/packages/agentrun-client/dev/local-smoke.ts`:
- Around line 11-15: Update the target configuration’s environment fallbacks for
PORT and GOOSE_SERVER__SECRET_KEY to use || instead of ??, so empty-string
values select the existing defaults before port conversion or secret assignment.
In `@clients/packages/hub-shim/src/server.ts`:
- Around line 374-387: Update the application-identity credential injection in
the credentialSources loop to mark the generated secretRef as optional, matching
the optional secret reference behavior in resolveModels(). Preserve the existing
identitySecret check and skip/log behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12d8f2e5-617d-4e8b-a085-a0c0ad36f38d
⛔ Files ignored due to path filters (5)
clients/harness-mock/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentic-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentrun-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/hub-shim/package-lock.jsonis excluded by!**/package-lock.jsonclients/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (68)
README.mdchanges/unreleased/clients-reference-stack.yamlclients/.gitignoreclients/README.mdclients/deploy/README.mdclients/deploy/gateway/Dockerfileclients/deploy/manifests/gateway.yamlclients/deploy/manifests/ingress.example.yamlclients/deploy/manifests/kustomization.yamlclients/deploy/manifests/ui.yamlclients/deploy/ui/Dockerfileclients/deploy/ui/nginx.confclients/docs/DEMO.mdclients/docs/demo/real-run.yamlclients/docs/demo/skill-probe.yamlclients/hack/demo-check.shclients/hack/demo-down.shclients/hack/demo-up.shclients/harness-goose/Dockerfileclients/harness-goose/entrypoint.shclients/harness-mock/Dockerfileclients/harness-mock/package.jsonclients/harness-mock/server.mjsclients/manifests/goose-bedrock.yamlclients/manifests/samples.yamlclients/packages/agentic-client/dev/selfcheck.tsclients/packages/agentic-client/package.jsonclients/packages/agentic-client/src/acp/index.tsclients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentic-client/tsconfig.jsonclients/packages/agentrun-client/dev/demo.tsclients/packages/agentrun-client/dev/local-smoke.tsclients/packages/agentrun-client/package.jsonclients/packages/agentrun-client/src/acp.tsclients/packages/agentrun-client/src/index.tsclients/packages/agentrun-client/src/kube.tsclients/packages/agentrun-client/src/portforward.tsclients/packages/agentrun-client/src/types.tsclients/packages/agentrun-client/tsconfig.jsonclients/packages/hub-shim/dev/browser-smoke.tsclients/packages/hub-shim/dev/dial-check.tsclients/packages/hub-shim/dev/drop-check.tsclients/packages/hub-shim/package.jsonclients/packages/hub-shim/src/acp-dial.tsclients/packages/hub-shim/src/server.tsclients/packages/hub-shim/tsconfig.jsonclients/ui/.gitignoreclients/ui/README.mdclients/ui/index.htmlclients/ui/package.jsonclients/ui/src/App.tsxclients/ui/src/app.cssclients/ui/src/components/ChatPanel.tsxclients/ui/src/components/CreateRunModal.tsxclients/ui/src/components/PhaseLabel.tsxclients/ui/src/components/RunDetailPage.tsxclients/ui/src/components/RunsPage.tsxclients/ui/src/format.tsclients/ui/src/main.tsxclients/ui/src/vite-env.d.tsclients/ui/tsconfig.app.jsonclients/ui/tsconfig.jsonclients/ui/tsconfig.node.jsonclients/ui/vite.config.tsdocs/adr/0004-client-contract-and-transports.mddocs/adr/0005-platform-resolved-params.md
🚧 Files skipped from review as they are similar to previous changes (51)
- clients/ui/vite.config.ts
- clients/packages/agentic-client/src/index.ts
- clients/harness-mock/package.json
- clients/docs/demo/real-run.yaml
- clients/ui/src/vite-env.d.ts
- clients/deploy/ui/nginx.conf
- clients/ui/package.json
- clients/.gitignore
- clients/packages/agentrun-client/package.json
- clients/ui/index.html
- clients/packages/agentrun-client/src/index.ts
- clients/ui/src/components/PhaseLabel.tsx
- clients/README.md
- clients/ui/tsconfig.node.json
- clients/ui/tsconfig.app.json
- clients/ui/src/format.ts
- clients/ui/.gitignore
- clients/docs/demo/skill-probe.yaml
- clients/packages/agentic-client/package.json
- changes/unreleased/clients-reference-stack.yaml
- README.md
- clients/ui/src/App.tsx
- clients/ui/src/main.tsx
- clients/packages/agentrun-client/src/portforward.ts
- clients/packages/hub-shim/dev/dial-check.ts
- clients/deploy/manifests/ingress.example.yaml
- clients/packages/agentrun-client/tsconfig.json
- clients/hack/demo-check.sh
- clients/packages/hub-shim/tsconfig.json
- clients/packages/agentrun-client/src/acp.ts
- clients/packages/agentic-client/tsconfig.json
- clients/ui/tsconfig.json
- clients/ui/README.md
- clients/packages/agentic-client/src/transport-shim/index.ts
- clients/packages/agentic-client/dev/selfcheck.ts
- clients/packages/hub-shim/src/acp-dial.ts
- clients/manifests/goose-bedrock.yaml
- clients/deploy/gateway/Dockerfile
- clients/manifests/samples.yaml
- clients/ui/src/components/RunsPage.tsx
- clients/hack/demo-down.sh
- clients/ui/src/components/RunDetailPage.tsx
- clients/packages/agentic-client/src/acp/index.ts
- clients/packages/agentrun-client/dev/demo.ts
- clients/ui/src/components/ChatPanel.tsx
- clients/ui/src/components/CreateRunModal.tsx
- clients/harness-mock/server.mjs
- clients/packages/hub-shim/dev/drop-check.ts
- clients/packages/agentrun-client/src/types.ts
- clients/packages/agentrun-client/src/kube.ts
- clients/packages/agentic-client/src/contract/index.ts
Comment on lines
+59
to
+84
| spec: | ||
| serviceAccountName: agentic-gateway | ||
| containers: | ||
| - name: gateway | ||
| image: agentic-gateway:dev | ||
| imagePullPolicy: IfNotPresent | ||
| ports: | ||
| - containerPort: 7080 | ||
| name: http | ||
| env: | ||
| - name: NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /healthz | ||
| port: http | ||
| initialDelaySeconds: 2 | ||
| periodSeconds: 5 | ||
| resources: | ||
| requests: | ||
| cpu: 50m | ||
| memory: 128Mi | ||
| limits: | ||
| memory: 512Mi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing securityContext hardening across both deploy Deployments. Neither agentic-gateway nor agentic-ui sets a pod/container securityContext, so both run with the default (root-capable, writable-rootfs, privilege-escalation-allowed) context. Same fix pattern applies to both; the gateway is higher priority since it also holds RBAC to read secrets and pods.
clients/deploy/manifests/gateway.yaml#L59-L84: addsecurityContext.runAsNonRoot: true+seccompProfile.type: RuntimeDefaultat pod level, andallowPrivilegeEscalation: false,readOnlyRootFilesystem: true,capabilities.drop: ["ALL"]on thegatewaycontainer.clients/deploy/manifests/ui.yaml#L21-L40: add the same pod-levelsecurityContext, andallowPrivilegeEscalation: false+capabilities.drop: ["ALL"]on theuicontainer (if addingreadOnlyRootFilesystem: true, mountemptyDirvolumes for nginx's writable dirs, e.g./var/cache/nginx,/tmp,/var/run).
📍 Affects 2 files
clients/deploy/manifests/gateway.yaml#L59-L84(this comment)clients/deploy/manifests/ui.yaml#L21-L40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clients/deploy/manifests/gateway.yaml` around lines 59 - 84, Harden both
Deployments: in clients/deploy/manifests/gateway.yaml lines 59-84, add pod-level
runAsNonRoot and RuntimeDefault seccompProfile, plus container-level
allowPrivilegeEscalation false, readOnlyRootFilesystem true, and drop ALL
capabilities for gateway; in clients/deploy/manifests/ui.yaml lines 21-40, add
the same pod-level settings and container-level privilege-escalation and
capability restrictions for ui, adding emptyDir mounts for nginx writable
directories if enabling readOnlyRootFilesystem.
Source: Linters/SAST tools
Comment on lines
+45
to
+54
| if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then | ||
| warn "building acp-mock-harness:dev" | ||
| (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) | ||
| fi | ||
| ok "acp-mock-harness:dev" | ||
| if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then | ||
| warn "building goose-harness:dev" | ||
| (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) | ||
| fi | ||
| ok "goose-harness:dev" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make minikube image build failures explicit, consistent with the npm install fix.
If either build fails, set -e will still abort the script, but with minikube's raw output instead of a clear die message — inconsistent with the || die pattern now used for npm install (lines 105-106, 128-129).
🩹 Proposed fix
if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then
warn "building acp-mock-harness:dev"
- (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null)
+ (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) \
+ || die "minikube image build failed for acp-mock-harness:dev"
fi
ok "acp-mock-harness:dev"
if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then
warn "building goose-harness:dev"
- (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null)
+ (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) \
+ || die "minikube image build failed for goose-harness:dev"
fi
ok "goose-harness:dev"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then | |
| warn "building acp-mock-harness:dev" | |
| (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) | |
| fi | |
| ok "acp-mock-harness:dev" | |
| if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then | |
| warn "building goose-harness:dev" | |
| (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) | |
| fi | |
| ok "goose-harness:dev" | |
| if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then | |
| warn "building acp-mock-harness:dev" | |
| (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) \ | |
| || die "minikube image build failed for acp-mock-harness:dev" | |
| fi | |
| ok "acp-mock-harness:dev" | |
| if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then | |
| warn "building goose-harness:dev" | |
| (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) \ | |
| || die "minikube image build failed for goose-harness:dev" | |
| fi | |
| ok "goose-harness:dev" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clients/hack/demo-up.sh` around lines 45 - 54, Update the minikube image
build commands in the acp-mock-harness and goose-harness blocks to use the same
explicit failure handling as the existing npm install commands, invoking die
with a clear, image-specific message when either build fails. Preserve the
current directory changes, build arguments, and success flow.
7 tasks
Read-only slice of konveyor#50. Shim serves agentplaybooks/agentplaybookruns through the generic READ_ONLY dispatch; agentic-client gains the AgentPlaybookRun contract types (no duration field — computed from timestamps; Ready=False/StageRunning documented as the healthy running state) and list/get methods. UI adds an Agent runs / Playbook runs toggle, a playbook-runs list (phase, x/y stages, computed duration), and a detail page with a ProgressStepper stage ladder where each stage links into the existing RunDetailPage via status.stages[].agentRunName (never recomputed — names hash-truncate past 63 chars). Stage-owned AgentRuns in the flat runs list get a parent-playbook badge and a disabled Delete (deleting one terminally fails the parent, konveyor#36). Verified against a live 3-stage playbook run (upgrade-run-1) on minikube with the merged konveyor#36 controllers. Refs konveyor#50. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
clients/ui/src/components/PlaybookRunDetailPage.tsx (2)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicate, inconsistent duration formatting between the playbook list and detail pages. Both render the same
playbookDuration()seconds value, but with different output for sub-minute durations (45svs0m45s), because the detail page reimplements the formatting inline instead of reusing the list page'sformatSeconds.
clients/ui/src/components/PlaybookRunDetailPage.tsx#L131-136: replace the inline${Math.floor(duration / 60)}m${...}expression with a call to the sharedformatSeconds(duration).clients/ui/src/components/PlaybookRunsPage.tsx#L30-35: exportformatSeconds(it currently isn't exported) soPlaybookRunDetailPage.tsxcan import it alongside the already-importedplaybookDuration.Proposed fix
--- a/clients/ui/src/components/PlaybookRunsPage.tsx +++ b/clients/ui/src/components/PlaybookRunsPage.tsx @@ -function formatSeconds(seconds?: number): string { +export function formatSeconds(seconds?: number): string {--- a/clients/ui/src/components/PlaybookRunDetailPage.tsx +++ b/clients/ui/src/components/PlaybookRunDetailPage.tsx @@ -import { playbookDuration } from "./PlaybookRunsPage"; +import { formatSeconds, playbookDuration } from "./PlaybookRunsPage"; @@ - <DescriptionListDescription> - {duration !== undefined ? `${Math.floor(duration / 60)}m${String(duration % 60).padStart(2, "0")}s` : "—"} - </DescriptionListDescription> + <DescriptionListDescription>{formatSeconds(duration)}</DescriptionListDescription>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/ui/src/components/PlaybookRunDetailPage.tsx` at line 1, Export the existing formatSeconds helper from PlaybookRunsPage, then import and reuse it in PlaybookRunDetailPage wherever playbookDuration is displayed. Replace the detail page’s inline minute/second formatting with formatSeconds(duration) so sub-minute durations match the list page.
60-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePolling continues (as a no-op) indefinitely after
goneis set.Once
gonebecomestrue,refreshis recreated (dependency change) and the effect restartssetInterval, which now just no-ops every 2s forever instead of being torn down. Harmless given the guard, but the interval could simply not be (re)started oncegone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/ui/src/components/PlaybookRunDetailPage.tsx` around lines 60 - 75, Update the polling effect that invokes refresh so it does not create or restart its interval when gone is true. Ensure the effect depends on gone and cleans up any existing interval when the run becomes gone, while preserving normal polling behavior before that state.clients/ui/src/components/PlaybookRunsPage.tsx (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
playbookDuration/formatSecondslook correct;formatSecondsshould be exported for reuse.See consolidated comment —
PlaybookRunDetailPage.tsxreimplements the same seconds-to-Xm YYsformatting inline with slightly different (inconsistent) behavior for sub-minute durations, instead of importing this function the way it already importsplaybookDuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/ui/src/components/PlaybookRunsPage.tsx` around lines 22 - 35, Export the existing formatSeconds function alongside playbookDuration, then update PlaybookRunDetailPage to import and reuse it instead of maintaining its inline seconds formatting. Preserve the current formatSeconds behavior for undefined, sub-minute, and minute-plus durations.clients/ui/src/App.tsx (1)
73-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a switch/lookup instead of the 4-way nested ternary for readability.
The
view.kind === "list" ? ... : view.kind === "playbooks" ? ... : view.kind === "playbookDetail" ? ... : (...)chain is functionally correct but getting hard to scan. Aswitch (view.kind)(or a small per-kind render map) would read more clearly as more view kinds are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/ui/src/App.tsx` around lines 73 - 125, Refactor the nested view-kind ternary in the App component into a clearer switch or per-kind render lookup keyed by view.kind. Preserve the existing RunsPage, PlaybookRunsPage, PlaybookRunDetailPage, and RunDetailPage props and navigation callbacks, including the detail back-navigation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/ui/src/components/PlaybookRunDetailPage.tsx`:
- Around line 131-136: Update the Duration rendering in PlaybookRunDetailPage to
reuse the existing formatSeconds helper used by PlaybookRunsPage instead of
constructing an inline minutes-and-seconds string. Preserve the undefined
fallback of "—" while ensuring identical playbookDuration() values use the same
formatting in list and detail views.
- Around line 66-71: Update the 404 detection in the getPlaybookRun error
handling catch block to match the specific HTTP 404 status, using the same
stricter “HTTP 404” check as RunDetailPage; retain the existing not-found
message check and setFetchError behavior.
---
Nitpick comments:
In `@clients/ui/src/App.tsx`:
- Around line 73-125: Refactor the nested view-kind ternary in the App component
into a clearer switch or per-kind render lookup keyed by view.kind. Preserve the
existing RunsPage, PlaybookRunsPage, PlaybookRunDetailPage, and RunDetailPage
props and navigation callbacks, including the detail back-navigation behavior.
In `@clients/ui/src/components/PlaybookRunDetailPage.tsx`:
- Line 1: Export the existing formatSeconds helper from PlaybookRunsPage, then
import and reuse it in PlaybookRunDetailPage wherever playbookDuration is
displayed. Replace the detail page’s inline minute/second formatting with
formatSeconds(duration) so sub-minute durations match the list page.
- Around line 60-75: Update the polling effect that invokes refresh so it does
not create or restart its interval when gone is true. Ensure the effect depends
on gone and cleans up any existing interval when the run becomes gone, while
preserving normal polling behavior before that state.
In `@clients/ui/src/components/PlaybookRunsPage.tsx`:
- Around line 22-35: Export the existing formatSeconds function alongside
playbookDuration, then update PlaybookRunDetailPage to import and reuse it
instead of maintaining its inline seconds formatting. Preserve the current
formatSeconds behavior for undefined, sub-minute, and minute-plus durations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f589f55-ec30-4b39-b310-4980030df577
📒 Files selected for processing (8)
clients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentrun-client/src/types.tsclients/packages/hub-shim/src/server.tsclients/ui/src/App.tsxclients/ui/src/components/PlaybookRunDetailPage.tsxclients/ui/src/components/PlaybookRunsPage.tsxclients/ui/src/components/RunsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- clients/packages/agentic-client/src/transport-shim/index.ts
- clients/packages/agentrun-client/src/types.ts
- clients/ui/src/components/RunsPage.tsx
- clients/packages/hub-shim/src/server.ts
Comment on lines
+66
to
+71
| } catch (err) { | ||
| const msg = errorMessage(err); | ||
| if (msg.includes("404") || msg.toLowerCase().includes("not found")) { | ||
| setGone(true); | ||
| } | ||
| setFetchError(msg); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Loose "404" substring match can misclassify unrelated errors as "run deleted".
errorMessage(err) for a failed getPlaybookRun call includes the full request URL (which embeds name) followed by HTTP ${status} (per ShimClient.send()). If a playbook run's name happens to contain the digits 404 (e.g. a generated suffix), any unrelated failure — a 500, a network blip — will match this check and permanently set gone, which short-circuits refresh() on every future poll (if (inFlight.current || gone) return;), leaving the user stuck on a false "no longer exists" screen with no way to recover short of a page reload. RunDetailPage's equivalent check uses the stricter msg.includes("HTTP 404"), which doesn't have this collision risk.
Proposed fix
} catch (err) {
const msg = errorMessage(err);
- if (msg.includes("404") || msg.toLowerCase().includes("not found")) {
+ if (msg.includes("HTTP 404")) {
setGone(true);
}
setFetchError(msg);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| const msg = errorMessage(err); | |
| if (msg.includes("404") || msg.toLowerCase().includes("not found")) { | |
| setGone(true); | |
| } | |
| setFetchError(msg); | |
| } catch (err) { | |
| const msg = errorMessage(err); | |
| if (msg.includes("HTTP 404")) { | |
| setGone(true); | |
| } | |
| setFetchError(msg); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clients/ui/src/components/PlaybookRunDetailPage.tsx` around lines 66 - 71,
Update the 404 detection in the getPlaybookRun error handling catch block to
match the specific HTTP 404 status, using the same stricter “HTTP 404” check as
RunDetailPage; retain the existing not-found message check and setFetchError
behavior.
Comment on lines
+131
to
+136
| <DescriptionListGroup> | ||
| <DescriptionListTerm>Duration</DescriptionListTerm> | ||
| <DescriptionListDescription> | ||
| {duration !== undefined ? `${Math.floor(duration / 60)}m${String(duration % 60).padStart(2, "0")}s` : "—"} | ||
| </DescriptionListDescription> | ||
| </DescriptionListGroup> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duration formatting diverges from PlaybookRunsPage's formatSeconds for the same data.
For durations under 60s, this inlines 0m45s while formatSeconds (used on the playbook list page for the identical playbookDuration() value) renders 45s. Same underlying data, inconsistent presentation between the list and detail views. See consolidated comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clients/ui/src/components/PlaybookRunDetailPage.tsx` around lines 131 - 136,
Update the Duration rendering in PlaybookRunDetailPage to reuse the existing
formatSeconds helper used by PlaybookRunsPage instead of constructing an inline
minutes-and-seconds string. Preserve the undefined fallback of "—" while
ensuring identical playbookDuration() values use the same formatting in list and
detail views.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Imports the working client stack prototyped in ibolton336/agentcontroller-client (the running system behind the contract proposal on #22), cleaned of POC scaffolding, as
clients/— plus ADRs 0004/0005 continuing this repo's ADR sequence.What this is
A verified, end-to-end client-side implementation of this repo's AgentRun/ACP contract, developed against the live controller on minikube (Agent Sandbox v0.5.0) with both a deterministic mock agent and a real goose+Bedrock agent. Each piece is the reference for a planned stream:
clients/packages/hub-shimclients/packages/agentic-client+clients/uiAcpSession, HTTP transport) + PatternFly SPA with streaming chat and HITL permission round-tripsclients/packages/agentrun-clientclients/harness-gooseKONVEYOR_*env contract (clone, model env mapping, prompt hints)clients/harness-mockTEST_PERMISSION,TEST_CANCEL,TEST_DROP) — useful as an e2e fixtureclients/deployclients/docs/DEMO.mdis the narrated end-to-end runbook (browser create → real goose+Bedrock agent → IDE attach/handoff → connection-drop resilience);clients/hack/demo-check.shsmokes the full ACP round-trip against a live cluster.What was cleared out
Relative to the prototype repo: the controller simulator and vendored CRD copies (this repo's reconcilers are the real thing now), the rendered controller install snapshot, prepared upstream patch files (delivered separately as PRs), simulator-era docs, and posted issue-comment drafts.
Verification
npm install+ typecheck/build clean in the new location; the client core's protocol selfcheck passes 17/17.Notes for reviewers
status.sandboxName, ACP Secret data keysecret-key, headless portless Service, whole-spec immutability) — worth a close read as the stream-2 handover spec.Summary by CodeRabbit